Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Output correct css when using css function in max(). #3708

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lumburr
Copy link
Contributor

@lumburr lumburr commented Mar 31, 2022

What: fix #3604

Why:

How:
For the following ast node

[Dimension, Expression]

less will skip non-Dimension nodes
https://github.com/less/less.js/blob/master/packages/less/src/less/functions/number.js#L26-L31
So, max(1, calc(1 + 1)) is treated as max(1).

We should not skip these nodes.
Checklist:

  • Documentation
  • Added/updated unit tests
  • Code complete

@iChenLei
Copy link
Member

Thanks for contribution.

@matthew-dean
Copy link
Member

So, max(1, calc(1 + 1)) is treated as max(1).

That doesn't seem correct. max(1, calc(1 + 1)) should output max(1, calc(1 + 1)) in the CSS. Similar for min(). See other function escaping code for functions like rgb() which output as-is if something like a var() statement is used.

@lumburr
Copy link
Contributor Author

lumburr commented Apr 24, 2022

In rgb((0 128 var(--num)), the program tries to convert var(--number) into a number,Throws an exception if the conversion cannot be performed.

function number(n) {
if (n instanceof Dimension) {
return parseFloat(n.unit.is('%') ? n.value / 100 : n.value);
} else if (typeof n === 'number') {
return n;
} else {
throw {
type: 'Argument',
message: 'color functions take numbers as parameters'
};
}
}

So, we also throw an exception on non-Dimension value.
Since the node was not successfully converted, Less will be output as is

@matthew-dean
Copy link
Member

@lumburr That's true but this will output as-is if compiled in Less.

.rule {
  color: rgb(0 128 var(--num));
}

So I'm saying, this in Less:

.rule {
  value: max(1, calc(1 + 1));
  }

...should output:

.rule {
   value: max(1, calc(1 + 1));
 }

@lumburr
Copy link
Contributor Author

lumburr commented Apr 25, 2022

Ohh,@matthew-dean I may not have expressed it clearly.
You are completely correct that in Less:

.rule {
  value: max(1, calc(1 + 1));
}

should output

.rule {
  value: max(1, calc(1 + 1));
}

And now the issue is caused because: max(1, calc(1 + 1)) is treated as max(1)

So what this PR does is to make Less output

.rule {
  value: max(1, calc(1 + 1));
}

correctly again.

@matthew-dean
Copy link
Member

Oops, can you resolve conflicts?

@bugron
Copy link

bugron commented Jan 12, 2023

Hi @lumburr, thanks for your contribution. Is there a chance you'll be able to fix merge conflicts here so that this PR can be merged?

@matthew-dean
Copy link
Member

@iChenLei Would you have time to resolve conflicts and add test cases for this issue to see if they are resolved?

@matthew-dean
Copy link
Member

Or @lumburr do you have time to do this?

@SoonIter
Copy link
Contributor

@matthew-dean
I've finished this in another pr #4266, could you CR and merge it? 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Less confused about max() + calc()
5 participants